feat: trace watchable messages #7403
Conversation
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
| } | ||
|
|
||
| // EnvoyGatewayTraces defines control plane tracing configurations. | ||
| type EnvoyGatewayTraces struct { |
There was a problem hiding this comment.
In EG, we use tracing in other place, better to keep consistency.
gateway/api/v1alpha1/envoygateway_types.go
Line 524 in 4c36666
There was a problem hiding this comment.
I didn't understand your point.
There was a problem hiding this comment.
the name should be EnvoyGatewayTracing instead of EnvoyGatewayTraces.
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (46.75%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7403 +/- ##
==========================================
- Coverage 72.39% 72.28% -0.11%
==========================================
Files 233 234 +1
Lines 34343 34477 +134
==========================================
+ Hits 24862 24922 +60
- Misses 7704 7766 +62
- Partials 1777 1789 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
thanks for working on this @shreealt is it possible to break this feature up into 2
|
internal/xds/runner/runner.go
Outdated
| parentCtx = update.Value.Context | ||
| } | ||
|
|
||
| _, span := tracer.Start(parentCtx, "XdsRunner.subscribeAndTranslate") |
There was a problem hiding this comment.
can we add another child span to r.cache.GenerateNewSnapshot and use the trace id as the version inside it, so this id trickles into envoy proxy
There was a problem hiding this comment.
done, but I hope this doesn't come off as a surprise to users?
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
hey @shreealt can you add some relevant before/after logs to the PR |
done. |
|
/retest |
internal/gatewayapi/runner/runner.go
Outdated
| var backendTLSPolicyStatusCount, clientTrafficPolicyStatusCount, backendTrafficPolicyStatusCount int | ||
| var securityPolicyStatusCount, envoyExtensionPolicyStatusCount, backendStatusCount, extensionServerPolicyStatusCount int | ||
|
|
||
| span.AddEvent("gateway_resources_translation_cycle", trace.WithAttributes(attribute.Int("resources.count", len(*val)))) |
There was a problem hiding this comment.
thoughts on just translate ? , since tracer already has gateway-api runner info
There was a problem hiding this comment.
sorry I did not get you, did you intend to name this event just translate?
internal/xds/runner/runner.go
Outdated
|
|
||
| // Add span attributes for observability | ||
| span.SetAttributes( | ||
| attribute.String("controller.key", update.Key), |
internal/logging/log.go
Outdated
| fields := []interface{}{ | ||
| "trace_id", sc.TraceID().String(), | ||
| "span_id", sc.SpanID().String(), | ||
| "trace_flags", sc.TraceFlags().String(), |
There was a problem hiding this comment.
imo not needed for logging purposes
| defer span.End() | ||
|
|
||
| span.SetAttributes( | ||
| attribute.String("controller.key", update.Key), |
internal/cmd/server.go
Outdated
| runner: metrics.New(cfg), | ||
| }, | ||
| { | ||
| // Start the Traces Server |
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
/retest |
|
tests are failing |
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
whoops, fixed it sorry. |
| } else { | ||
| // Translate to ratelimit xDS Config. | ||
| rvt, err := r.translate(update.Value) | ||
| rvt, err := r.translate(update.Value.XdsIR) |
There was a problem hiding this comment.
can we also add a span to this
|
hey @shreealt, added a few more minor comments, but this is close to being merged. since this PR touches the watchable messages, can you run a test locally to prove that pushing identical messages dont cause churn (watchable subscribes)
|
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
@arkodg here are a few metrics. 
|
|
same test on main branch: 
|
Signed-off-by: Shreemaan Abhishek <[email protected]>
Co-authored-by: Arko Dasgupta <[email protected]> Signed-off-by: shreealt <[email protected]>
5 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #7324
Release Notes: Yes/No
Logs
Before:
After: